Simplify shardsWithState #91991
Simplify shardsWithState #91991
Conversation
idegtiarenko
added
>non-issue
:Distributed Coordination/Allocation
|
Pinging @elastic/es-distributed (Team:Distributed) |
| if (shardEntry.state() == state) { | ||
| shards.add(shardEntry); | ||
| } | ||
| } |
There was a problem hiding this comment.
This branch was effectively unreachable.
| } | ||
| } | ||
| return shards; | ||
| } | ||
|
|
||
| private LinkedHashSet<ShardRouting> getShardsWithState(ShardRoutingState state) { | ||
| return switch (state) { | ||
| case UNASSIGNED -> throw new IllegalArgumentException("Unassigned shards are not linked to a routing node"); |
There was a problem hiding this comment.
This was previously returning empty list or 0 but I guess it is better to be explicit that routing node can not be used to obtain unassigned shards.
There was a problem hiding this comment.
These methods are generally rather inefficient, which is kind of ok because they're mostly only used in tests. Still I think it's somewhat dangerous to have such inefficient stuff in the prod implementation where they risk being used somewhere important in future. Can we try and avoid copying to lists in prod, maybe moving to a Stream<ShardRouting> based API where applicable?
| @@ -215,25 +218,11 @@ void remove(ShardRouting shard) { | |||
| * @return number of shards | |||
| */ | |||
| public int numberOfShardsWithState(ShardRoutingState... states) { | |||
There was a problem hiding this comment.
Can we inline this? It's only used in a handful of tests.
| } | ||
| } | ||
| return shards; | ||
| } | ||
|
|
||
| private LinkedHashSet<ShardRouting> getShardsWithState(ShardRoutingState state) { |
There was a problem hiding this comment.
IMO it's confusing to have getShardsWithState and shardsWithState which do quite different things (in terms of copying the internal value or not). How about naming this something like internalGetShardsWithState?
| @@ -242,20 +231,7 @@ public int numberOfShardsWithState(ShardRoutingState... states) { | |||
| * @return List of shards | |||
| */ | |||
| public List<ShardRouting> shardsWithState(ShardRoutingState state) { | |||
There was a problem hiding this comment.
AFAICT this method is only used in two places in prod code, both of which could reasonably accept a Stream<ShardRouting> instead and avoid the need to copy into a new list.
There was a problem hiding this comment.
This has plenty of unit test usages that needs to be updated as well. Do you mind if I do it in a followup pr (possibly with moving some of this methods to a test code)?
| @@ -96,7 +96,6 @@ public void testRemove() { | |||
| } | |||
|
|
|||
| public void testNumberOfShardsWithState() { | |||
| assertThat(routingNode.numberOfShardsWithState(ShardRoutingState.INITIALIZING, ShardRoutingState.STARTED), equalTo(2)); | |||
There was a problem hiding this comment.
No need to test since this method is removed
| @@ -507,10 +506,14 @@ public void testRebalanceFailure() { | |||
| RoutingNodes routingNodes = clusterState.getRoutingNodes(); | |||
|
|
|||
| assertThat(clusterState.routingTable().index("test").size(), equalTo(2)); | |||
| assertThat(routingNodes.node("node1").numberOfShardsWithState(STARTED, RELOCATING), equalTo(2)); | |||
| assertThat(routingNodes.node("node1").numberOfShardsWithState(STARTED), lessThan(3)); | |||
There was a problem hiding this comment.
This and following lessThan(3) assertions are removed as they are covered by assertion above
* upstream/main: (209 commits) Remove unused methods and classes from HLRC (elastic#92030) Clean up on exception while chunking XContent (elastic#92024) Add profiling plugin (elastic#91640) Remove unused methods and classes from HLRC (elastic#92012) Remove IndexerState from HLRC (elastic#92023) Ensure cached time elapses in ClusterServiceIT (elastic#91986) Chunked encoding for RestGetIndicesAction (elastic#92016) Simplify shardsWithState (elastic#91991) [DOCS] Updates ML decider docs by mentioning CPU as scaling criterion (elastic#92018) Add chunking to ClusterState.Custom impls (elastic#91963) Speedup time_series agg by caching current tsid ordinal, parent bucket ordinal and buck ordinal (elastic#91784) Drop the ingest listener call count tracking (elastic#92003) [DOCS] fixes issue number 91889 - missing [discrete] header (elastic#91976) Fix PersistentTasksClusterServiceTests (elastic#92002) [docs] Update search-settings documentation to reflect the fact that the indices.query.bool.max_clause_count setting has been deprecated (elastic#91811) Clarify writability in Netty4HttpPipeliningHandler (elastic#91982) Load stable plugins as synthetic modules (elastic#91869) Handle any exception thrown while generating source for an IngestDocument (elastic#91981) fixing Apache HttpHost url on java-rest doc (elastic#91945) Implement repair functionality for aliases colliding with indices bug (elastic#91887) ...
This simplifies shardsWithState by removing the dead code.